fix(types): use explicit .js extensions in relative imports - #770
fix(types): use explicit .js extensions in relative imports#770tyler-reitz wants to merge 1 commit into
Conversation
The emitted .d.ts inherited source's extensionless relative specifiers (`from './useObservable'`, and `from '.'` in two files), which node16 and nodenext resolution reject. Consumers on those settings hit resolution errors reading our declarations, while the runtime bundles were fine, so nothing in the build surfaced it. `moduleResolution: bundler` accepts both forms in source, so this is a no-op for the build and for bundler consumers. Verified with @arethetypeswrong/cli against the packed build, with no ignore rules: before node16 (from ESM) internal resolution error after node16 (from ESM) pass node16 (from CJS) still reports "no types" on this branch; that is the missing `types` export condition, fixed separately in FirebaseExtended#766. With both changes applied, all four resolution modes pass with no suppression. Fixes FirebaseExtended#769.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
armando-navarro
left a comment
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks Tyler, and thanks for filing #769 out of the #766 discussion. This is the fix I was hoping for, and it approves cleanly.
What I verified
I reproduced the problem on main and confirmed the fix against a fresh build, using attw with no ignore rules:
- On
main, node16-from-ESM resolves the declarations to an internal resolution error, and the emitted.d.tscarry 16 extensionless relative specifiers. - On this branch, node16-from-ESM goes green and zero extensionless relative specifiers remain in the emitted declarations.
- node16-from-CJS stays on "no types", which is the missing
typescondition and lands with #766, exactly as your description says. tsc --noEmitis clean on bothtsconfig.jsonandtsconfig.test.json, andvite buildsucceeds.- The runtime bundles (
dist/index.js,dist/index.umd.cjs) carry no relative specifiers, so this is resolved away at build time and nothing changes for runtime or for bundler consumers.
I also reverted a single specifier back to extensionless as a check, and the internal resolution error came straight back, so the fix is doing real work across all 29 edits, not riding on one.
One thing worth a follow-up (non-blocking)
The fix is correct, but I do not think it is self-enforcing yet. tsconfig.json stays on moduleResolution: bundler, which accepts both the extensionless and the .js forms, so tsc will stay green if an extensionless relative import creeps back in later. And there is no attw/publint job in CI today, so nothing catches a regression until #766 adds one.
Your own note on #769 floated moving moduleResolution in the same change so the compiler enforces this. Would you want to do that here, or add a lint rule for relative-import extensions, so this cannot silently regress before #766 lands? I have not checked whether switching resolution builds cleanly (it can have knock-on effects), so I am asking rather than recommending.
On the #766 suppression
Confirming the coordination in your description: with this branch plus #766's types condition, all four attw modes pass with no ignore rules. So whichever order these land, #766 can drop the .attw.json ignoreRules entry and the suppression never ships permanently. Landing this one first is the tidier path, as you say.
Small note
src/functions.tsx now has two separate imports that both read from './index.js' (they were from './' and from '.' before). Totally valid, just foldable into one line if you feel like it.
If any of this reads wrong, say so and I will take another look.
2 participants
Summary
Fixes #769. Adds explicit
.jsextensions to relative import specifiers insrc, so the emitted.d.tsstop carrying extensionless paths thatnode16/nodenextresolution rejects.Source-only change, 10 files, all mechanical.
moduleResolution: bundleraccepts both forms, so nothing changes for the build or for bundler consumers.The bug
TypeScript emits relative specifiers unchanged, so
export * from './auth'insrc/index.tsbecomesfrom './auth'indist/index.d.ts. Undernode16that is not a valid specifier, and a consumer on"moduleResolution": "node16"gets resolution errors reading our declarations.Two shapes were involved:
.d.tsfiles (from './useObservable')from './'andfrom '.', inauth,database,firestore,functions,remote-config,sdk,storageanduseObservable. These resolve to the barrel and now point at./index.jsexplicitlyThe runtime bundles resolve fine, which is why nothing in the build caught it.
Verification
@arethetypeswrong/cliagainst the packed build, with no ignore rules:The remaining
node16 (from CJS)failure is a different bug: notypescondition on eitherexportsbranch, fixed in #766. Applying both, all four modes pass with no suppression, which is the acceptance test #769 asks for.Also confirmed:
tsc --noEmitclean on bothtsconfig.jsonandtsconfig.test.json,vite buildsucceeds, and zero extensionless relative specifiers remain in the emitted declarations.Relationship to #766
#766 adds
attwto CI and currently suppresses this via.attw.json:{ "ignoreRules": ["internal-resolution-error"] }That ignore is rule-global, so it hides any future error of the class. If this lands first, #766 can drop the ignore before it merges and the suppression never ships. If #766 lands first, deleting the entry becomes this issue's acceptance test instead. Either order works; this one is tidier.
Test plan
npm testnpx tsc --noEmiton both tsconfigsattwon the packed build shows no internal resolution error